Avoid re-assigning the global pid for client backends and bg workers when the application_name changes #7791
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #7791 +/- ##
==========================================
- Coverage 89.70% 89.70% -0.01%
==========================================
Files 283 283
Lines 60509 60516 +7
Branches 7541 7542 +1
==========================================
+ Hits 54278 54284 +6
+ Misses 4076 4075 -1
- Partials 2155 2157 +2 |
|
AssignGlobalPID() deals with two kinds of backends:
And we may need to perform catalog access for the class-1, not for class-2. This is because, for class-1, we make use of the local node id to actually generate a global pid (GenerateGlobalPID()). However for class-2, we simply extract the global pid inherited from the originating backend from the application_name specified by originating backend when openning that connection and extracting the global pid from application_name doesn't require catalog access at all (ExtractGlobalPID()). And the nice part is that for class-I, the global pid doesn't change when application_name changes. For this reason, rather than such a code change that this PR proposed earlier, only taking an action for class-2 looks like a more appropriate fix. |
onurctirtir
changed the title
…id without unsafe catalog access
onurctirtir
force-pushed
the
fix-unsafe-reassign-global-pid
branch
from
a870bca to
9f2f0e3
Compare
…al node id without unsafe catalog access" This reverts commit d06c0cd.
…ation_name changes
onurctirtir
force-pushed
the
fix-unsafe-reassign-global-pid
branch
from
9f2f0e3 to
1bd0ed8
Compare
Ok, so looks like, we rely on ApplicationNameAssignHook to assign global pid for external client backends as well even though such cases are quite rare, so I need to revert back to the initial design. |
onurctirtir
changed the title
onurctirtir
changed the title
Fixed that failing test too, so decided to move forward with the alternative approach documented again. |
|
LGTM. |
…when the application_name changes (#7791) DESCRIPTION: Fixes a crash that happens because of unsafe catalog access when re-assigning the global pid after application_name changes. When application_name changes, we don't actually need to try re-assigning the global pid for external client backends because application_name doesn't affect the global pid for such backends. Plus, trying to re-assign the global pid for external client backends would unnecessarily cause performing a catalog access when the cached local node id is invalidated. However, accessing to the catalog tables is dangerous in certain situations like when we're not in a transaction block. And for the other types of backends, i.e., the Citus internal backends, we need to re-assign the global pid when the application_name changes because for such backends we simply extract the global pid inherited from the originating backend from the application_name -that's specified by originating backend when openning that connection- and this doesn't require catalog access. (cherry picked from commit 7341191)
…when the application_name changes (#7791) DESCRIPTION: Fixes a crash that happens because of unsafe catalog access when re-assigning the global pid after application_name changes. When application_name changes, we don't actually need to try re-assigning the global pid for external client backends because application_name doesn't affect the global pid for such backends. Plus, trying to re-assign the global pid for external client backends would unnecessarily cause performing a catalog access when the cached local node id is invalidated. However, accessing to the catalog tables is dangerous in certain situations like when we're not in a transaction block. And for the other types of backends, i.e., the Citus internal backends, we need to re-assign the global pid when the application_name changes because for such backends we simply extract the global pid inherited from the originating backend from the application_name -that's specified by originating backend when openning that connection- and this doesn't require catalog access. (cherry picked from commit 7341191)
DESCRIPTION: Fixes a crash that happens because of unsafe catalog access when re-assigning the global pid after application_name changes.
When application_name changes, we don't actually need to
try re-assigning the global pid for external client backends because
application_name doesn't affect the global pid for such backends. Plus,
trying to re-assign the global pid for external client backends would
unnecessarily cause performing a catalog access when the cached local
node id is invalidated. However, accessing to the catalog tables is
dangerous in certain situations like when we're not in a transaction
block. And for the other types of backends, i.e., the Citus internal
backends, we need to re-assign the global pid when the application_name
changes because for such backends we simply extract the global pid
inherited from the originating backend from the application_name -that's
specified by originating backend when openning that connection- and this
doesn't require catalog access.